Use PSR-6 caching interfce & remove desarolla2 cache #10

sunspikes · 2017-10-27T13:52:54Z

Use PSR-6 caching interfaces
Remove Dsarrola cache adapter and use php-cache (for functional tests)

sunspikes · 2017-10-27T14:08:35Z

Can you please have a look. So basically i removed Desarrola completely and decoupled the caching part (it now depends only on PSR-6 interfaces), for functional tests I'm using the array cache from php-cache and I've started using PHP 7 typehints.

Now for v2 i think,

Add back unit tests for throttling algorithms
Need to enforce strict types and type hints in all places (I think nullable typehints and void types would have been better, but for this min version need to be bumped to 7.1)
Refactor the throttling algorithms a bit to reduce duplicate code & improve the documentation a bit more

That's what i can think of now, what you think ?

Feijs

Can you please have a look. So basically i removed Desarrola completely and decoupled the caching part (it now depends only on PSR-6 interfaces), for functional tests I'm using the array cache from php-cache and I've started using PHP 7 typehints.

Looks good! 👍 I've added a few (mostly minor) notes

Add back unit tests for throttling algorithms

Why have these been removed? I can update them to match your changes in this PR if that is the issue?

Need to enforce strict types and type hints in all places (I think nullable typehints and void types would have been better, but for this min version need to be bumped to 7.1)

Refactor the throttling algorithms a bit to reduce duplicate code & improve the documentation a bit more

Agreed, perhaps an incremental upgrade to 7.1 would be best, but since active support for 7.0 ends in a month, it it quite reasonable to update to 7.1 in one go

Feijs · 2017-10-30T11:11:25Z

build/logs/clover.xml

@@ -0,0 +1,3294 @@
+<?xml version="1.0" encoding="UTF-8"?>


Don't think this should have been submitted?

Ouch!, removed, thanks!

Feijs · 2017-10-30T12:27:26Z

src/Cache/ThrottlerCache.php

+     */
+    public function getItem(string $key): ThrottlerItemInterface
+    {
+        $item = $this->getThrottlerItem($key);


Can be returned inline

Feijs · 2017-10-30T12:49:13Z

src/Cache/ThrottlerCache.php

+    }
+
+    /**
+     * @param string $key


These docblocks (for the public methods) can all be replaced by @inheritdoc

Feijs · 2017-10-30T12:51:21Z

src/Cache/ThrottlerCache.php

+     * @throws CacheAdapterException
+     * @throws ItemNotFoundException
+     */
+    private function getThrottlerItem(string $key): ThrottlerItemInterface


Both private methods are used only once, so could be moved to the corresponding public method

Yes, thanks!

Feijs · 2017-10-30T12:51:39Z

src/Cache/ThrottlerCache.php

+    /**
+     * @param string $key
+     *
+     * @return ThrottlerItemInterface


I would add these exceptions to the interface

Yes, that makes sense, thanks!

Feijs · 2017-10-30T13:47:20Z

src/Cache/ThrottlerCache.php

+            $cacheItem = $this->cacheItemPool->getItem($key);
+
+            if ($cacheItem->isHit()) {
+                $this->cacheItemPool->deleteItem($key);


Why is the item deleted if it already exists? Is there an issue with updates if the item pre-existed?

Yes, when i tested (with redis) i see the item gets duplicated. this way it works, but may be we can add more real tests

Ok, so i got the problem, it was because all tests were using same key. I've fixed this issue and I removed the delete line. And now the tests are running on redis by default (if available)

Feijs · 2017-10-30T13:49:25Z

src/Throttle/Entity/CacheCount.php

+    /** @var int $count */
+    private $count;
+
+    /** @var int $ttl */


$ttl can also be null

Feijs · 2017-10-30T14:00:28Z

src/Throttle/Throttler/ElasticWindowThrottler.php

@@ -101,7 +104,9 @@ public function count()
        }

        try {
-            $this->counter = $this->cache->get($this->key);
+            /** @var CacheCount $item */


I think it would be a nice improvement to check whether these items contain the expected type:

if (!$item instanceof CacheCount) { $this->counter = 0; }

Theoretically the cache could contain garbage or unsafe data.

Perhaps something for a seperate PR though

Yeah, this can be done, but in case it is a hit, it should get the actual saved value (best case)

Feijs · 2017-10-30T14:36:58Z

src/Throttle/Throttler/ElasticWindowThrottler.php


-        $this->cache->set($this->key, $this->counter, $this->ttl);
+        $this->cache->setItem($this->key, $item);


The item variable is used only once, so this can be done inline. Applies to a number of similar cases below

Feijs · 2017-10-30T14:40:21Z

tests/Functional/AbstractThrottlerTestCase.php

@@ -46,6 +44,7 @@ public function testThrottlePostLimit()
            $throttle->hit();
        }

+


superfluous newline

sunspikes · 2017-11-07T17:05:20Z

Why have these been removed? I can update them to match your changes in this PR if that is the issue?

It would be awesome if you can add it back. Now all of them fails because of these new changes, i've tried to fix it but it was not good as i wanted it to be :D

@Feijs Also, thank you so much for your comments!

sunspikes · 2017-11-09T10:06:01Z

I will merge this to the release branch, if there is something to be addressed it can be done in a new PR.

Use PSR-6 caching interfce & remove desarolla2 cache

fcf7df7

sunspikes force-pushed the feature/psr-6 branch from ad84c44 to fcf7df7 Compare October 27, 2017 13:57

sunspikes changed the base branch from master to 2.x-dev October 27, 2017 14:08

sunspikes added the 2.x label Oct 27, 2017

Feijs reviewed Oct 30, 2017

View reviewed changes

Updated serialization logic

8bf2713

sunspikes force-pushed the feature/psr-6 branch from f780b9b to 8bf2713 Compare November 8, 2017 09:16

sunspikes added 2 commits November 8, 2017 10:41

Added missing license texts

0a7ad19

Upgrade to phpunit 6

6901f96

sunspikes force-pushed the feature/psr-6 branch from d22b9d5 to 6901f96 Compare November 8, 2017 11:02

Run the functional tests on redis

1e9dfb7

sunspikes force-pushed the feature/psr-6 branch from 16358df to 1e9dfb7 Compare November 8, 2017 15:57

Move the interface to abstract class

982bf1d

sunspikes merged commit 6f5f4d2 into 2.x-dev Nov 9, 2017

sunspikes deleted the feature/psr-6 branch November 9, 2017 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PSR-6 caching interfce & remove desarolla2 cache #10

Use PSR-6 caching interfce & remove desarolla2 cache #10

sunspikes commented Oct 27, 2017

sunspikes commented Oct 27, 2017

Feijs left a comment

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

sunspikes Nov 8, 2017

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

Feijs Oct 30, 2017

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

Feijs Oct 30, 2017

sunspikes Nov 7, 2017

sunspikes commented Nov 7, 2017 •

edited

Loading

sunspikes commented Nov 9, 2017


		$this->cache->set($this->key, $this->counter, $this->ttl);
		$this->cache->setItem($this->key, $item);

		@@ -46,6 +44,7 @@ public function testThrottlePostLimit()
		$throttle->hit();
		}

Use PSR-6 caching interfce & remove desarolla2 cache #10

Use PSR-6 caching interfce & remove desarolla2 cache #10

Conversation

sunspikes commented Oct 27, 2017

sunspikes commented Oct 27, 2017

Feijs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunspikes commented Nov 7, 2017 • edited Loading

sunspikes commented Nov 9, 2017

sunspikes commented Nov 7, 2017 •

edited

Loading